Fix: stellar contract info * commands require network when network no… #1676

Dhanraj30 · 2024-10-15T16:34:35Z

…t required
#1653

What

Additional logic for checking network requirement
If WASM file is provided, don't require network arguments

Why

The current implementation of the stellar contract info commands requires network parameters (e.g., --rpc-url, --network-passphrase, --network) even when users are working with local files such as WebAssembly (.wasm) files. This requirement can lead to unnecessary errors and confusion for users who are only looking to interact with local resources.

This change introduces conditional checks in the command handling logic to ensure that network parameters are only enforced when they are actually needed

Known limitations

User Documentation: Existing documentation may need updates to clarify the new behavior of the commands concerning local .wasm file interactions. It is essential to ensure users are aware that network parameters are optional in these cases.

…t required

cmd/soroban-cli/src/commands/network/mod.rs

cmd/soroban-cli/src/commands/global.rs

willemneal · 2024-10-22T13:08:18Z

Currently your solution isn't correct. Clap parses commands and runs the corresponding "run" method. So in the case of this PR stellar network's run method is called and then it will call the relevant subcommand. In this PR that method is called e.g. network add. Then when it returns your logic is called. So it has no effect on the behavior of the subcommand nor on stellar contract info's subcommands. The solution to this PR is to edit the info's shared fetch_wasm, which currently resolves the network even if the wasm` arg is used which is a path to a local Wasm file. So the solution is to first check if the wasm arg is used and if so return early with the bytes. Otherwise you will need the network for the other two cases. So please remove the changes you have made here and edit this function.

stellar-cli/cmd/soroban-cli/src/commands/contract/info/shared.rs

Lines 61 to 91 in 3f06e1d

    
           pub async fn fetch_wasm(args: &Args) -> Result<Option<Vec<u8>>, Error> { 
        
               let network = &args.network.get(&args.locator)?; 
        
               let wasm = if let Some(path) = &args.wasm { 
        
                   wasm::Args { wasm: path.clone() }.read()? 
        
               } else if let Some(wasm_hash) = &args.wasm_hash { 
        
                   let hash = hex::decode(wasm_hash) 
        
                       .map_err(|_| InvalidWasmHash(wasm_hash.clone()))? 
        
                       .try_into() 
        
                       .map_err(|_| InvalidWasmHash(wasm_hash.clone()))?; 
        
                   let hash = xdr::Hash(hash); 
        
                   let client = network.rpc_client()?; 
        
                   client 
        
                       .verify_network_passphrase(Some(&network.network_passphrase)) 
        
                       .await?; 
        
                   get_remote_wasm_from_hash(&client, &hash).await? 
        
               } else if let Some(contract_id) = &args.contract_id { 
        
                   let res = wasm::fetch_from_contract(contract_id, network, &args.locator).await; 
        
                   if let Some(ContractIsStellarAsset) = res.as_ref().err() { 
        
                       return Ok(None); 
        
                   } 
        
                   res? 
        
               } else { 
        
                   unreachable!("One of contract location arguments must be passed"); 
        
               }; 
        
               Ok(Some(wasm))

…wasm

willemneal

LGTM just some small nits for removing extra new lines

cmd/soroban-cli/src/commands/global.rs

cmd/soroban-cli/src/commands/network/mod.rs

cmd/soroban-cli/src/commands/contract/info/shared.rs

Co-authored-by: Willem Wyndham <[email protected]>

Ifropc · 2024-10-24T18:03:03Z

I'm pretty sure this won't work unless arguments are changed. From what I know Clap will parse arguments first, and we have network as one of the arguments, so I don't think this issue will be resolved with this PR

cargo run contract info interface --wasm ~/wasm/hello_world.wasm
    Finished `dev` profile [unoptimized + debuginfo] target(s) in 0.17s
     Running `target/debug/soroban contract info interface --wasm /home/ifro/wasm/hello_world.wasm`
error: the following required arguments were not provided:
  --rpc-url <RPC_URL>
  --network-passphrase <NETWORK_PASSPHRASE>
  --network <NETWORK>

…work

willemneal · 2024-11-01T13:48:02Z

@Ifropc I fixed it so that clap doesn't require network options at all. This should fix this and make commands like keys generate only complain if the RPC is required, e.g. --fund.

Ifropc · 2024-11-01T18:04:48Z

@willemneal I think we can go with that approach, the only downside is that now arguments are no longer validated by clap. So we now will need to make sure we do the proper validation and give a good error message (e.g. current error message is not descriptive enough)

$ cargo run keys generate foo
   Compiling soroban-cli v21.5.0 (/home/ifro/projects/stellar-cli/cmd/soroban-cli)
    Finished `dev` profile [unoptimized + debuginfo] target(s) in 10.34s
     Running `target/debug/soroban keys generate foo`
⚠️ Behavior of `generate` will change in the future, and it will no longer fund by default. If you want to fund please provide `--fund` flag. If you don't need to fund your keys in the future, ignore this warning. It can be suppressed with -q flag.
❌ error: network arg or rpc url and network passphrase are required if using the network

willemneal · 2024-11-01T23:27:53Z

@Ifropc FYI you can do cargo s which is the same as cargo run --quiet.

Ifropc · 2024-11-05T20:09:00Z

Fixed in #1698

willemneal · 2024-11-05T21:40:18Z

@Ifropc We still need this PR because it moves the network resolution after checking if there is a wasm arg.

Ifropc · 2024-11-05T21:42:58Z

Oh you are right sorry missed that

Fix: stellar contract info * commands require network when network no…

ab3dd8f

…t required

willemneal reviewed Oct 21, 2024

View reviewed changes

cmd/soroban-cli/src/commands/network/mod.rs Outdated Show resolved Hide resolved

Added wasm field to Args struct and updated Cmd::run logic

21e45f1

willemneal reviewed Oct 22, 2024

View reviewed changes

cmd/soroban-cli/src/commands/global.rs Outdated Show resolved Hide resolved

fix: prevent network resolution when local wasm is provided in fetch_…

824e43a

…wasm

willemneal approved these changes Oct 24, 2024

View reviewed changes

willemneal and others added 5 commits October 24, 2024 12:53

Merge branch 'main' into fix-contract-info-network

e3e2301

Update cmd/soroban-cli/src/commands/global.rs

b838367

Co-authored-by: Willem Wyndham <[email protected]>

Update cmd/soroban-cli/src/commands/network/mod.rs

2107c74

Co-authored-by: Willem Wyndham <[email protected]>

Update cmd/soroban-cli/src/commands/global.rs

7e8fffd

Co-authored-by: Willem Wyndham <[email protected]>

Update cmd/soroban-cli/src/commands/contract/info/shared.rs

6db64c4

Co-authored-by: Willem Wyndham <[email protected]>

Merge branch 'main' into fix-contract-info-network

8ddce4f

leighmcculloch requested a review from willemneal October 30, 2024 14:56

willemneal added 2 commits November 1, 2024 09:25

Merge remote-tracking branch 'origin/main' into fix-contract-info-net…

1156ace

…work

fix: make network args optional and postpone error until resolution

32ced65

Merge branch 'main' into fix-contract-info-network

bd08f00

janewang mentioned this pull request Nov 5, 2024

stellar contract info * commands require network when network not required #1653

Closed

Ifropc closed this Nov 5, 2024

willemneal reopened this Nov 5, 2024

willemneal approved these changes Nov 5, 2024

View reviewed changes

Ifropc enabled auto-merge (squash) November 5, 2024 21:43

Ifropc approved these changes Nov 5, 2024

View reviewed changes

Ifropc merged commit 3209130 into stellar:main Nov 5, 2024
50 checks passed

BrewTestBot mentioned this pull request Nov 19, 2024

stellar-cli 22.0.0 Homebrew/homebrew-core#198222

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix: stellar contract info * commands require network when network no… #1676

Fix: stellar contract info * commands require network when network no… #1676

Dhanraj30 commented Oct 15, 2024

willemneal commented Oct 22, 2024

willemneal left a comment

Ifropc commented Oct 24, 2024

willemneal commented Nov 1, 2024

Ifropc commented Nov 1, 2024

willemneal commented Nov 1, 2024

Ifropc commented Nov 5, 2024

willemneal commented Nov 5, 2024

Ifropc commented Nov 5, 2024

Fix: stellar contract info * commands require network when network no… #1676

Fix: stellar contract info * commands require network when network no… #1676

Conversation

Dhanraj30 commented Oct 15, 2024

What

Why

Known limitations

willemneal commented Oct 22, 2024

willemneal left a comment

Choose a reason for hiding this comment

Ifropc commented Oct 24, 2024

willemneal commented Nov 1, 2024

Ifropc commented Nov 1, 2024

willemneal commented Nov 1, 2024

Ifropc commented Nov 5, 2024

willemneal commented Nov 5, 2024

Ifropc commented Nov 5, 2024